Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(pageserver): make image split layer writer finish atomic #8841

Merged
merged 11 commits into from
Oct 21, 2024
Merged

fix(pageserver): make image split layer writer finish atomic #8841

merged 11 commits into from
Oct 21, 2024

Conversation

skyzh
Copy link
Member

@skyzh skyzh commented Aug 26, 2024

Problem

Part of #8836

Summary of changes

This pull request makes the image layer split writer atomic when finishing the layers. All the produced layers either finish at the same time, or discard at the same time. Note that this does not prevent atomicity when crash, but anyways, it will be cleaned up on pageserver restart.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@skyzh skyzh requested a review from a team as a code owner August 26, 2024 19:55
@skyzh skyzh requested a review from jcsp August 26, 2024 19:55
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 26, 2024 •
edited
Loading

5229 tests run: 5015 passed, 0 failed, 214 skipped (full report)

Flaky tests (1)

Postgres 15

Code coverage* (full report)

  • functions: 31.2% (7568 of 24249 functions)
  • lines: 48.8% (59936 of 122767 lines)

* collected from Rust tests only

The comment gets automatically updated with the latest test results
cc8f245 at 2024-10-21T14:34:45.764Z :recycle:

This was referenced Aug 30, 2024
Merged
Merged
@skyzh skyzh force-pushed the skyzh/split-layer-atomic branch from 298e40f to a99f9dd Compare September 16, 2024 00:25
orca-security-us[bot]
orca-security-us bot reviewed Sep 16, 2024
View reviewed changes
Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca

@skyzh skyzh requested a review from problame September 16, 2024 01:03
@skyzh skyzh force-pushed the skyzh/split-layer-atomic branch from 0bb64f2 to d333034 Compare September 18, 2024 14:27
This was referenced Sep 18, 2024
Merged
Open
@skyzh skyzh requested a review from problame September 18, 2024 15:37
@skyzh skyzh requested a review from jcsp September 23, 2024 14:32
@skyzh
Copy link
Member Author

skyzh commented Sep 23, 2024

comments resolved and ready for review again :)

This was referenced Sep 23, 2024
Closed
Open
@skyzh skyzh requested a review from problame September 24, 2024 14:43
problame
problame previously requested changes Sep 26, 2024
View reviewed changes
pageserver/src/tenant/storage_layer/split_writer.rs Outdated Show resolved Hide resolved
@skyzh skyzh requested a review from problame September 30, 2024 18:45
@skyzh
Copy link
Member Author

skyzh commented Sep 30, 2024

ready for review; the new commit also fixes the problem of image layer writer that it does not attempt to remove the temporary file if anything goes wrong

@skyzh skyzh requested a review from arpad-m October 17, 2024 15:42
@skyzh
Copy link
Member Author

skyzh commented Oct 17, 2024

@arpad-m do you mind have a final look of this PR since Christian is busy + this patch has been there for too long? Thanks a lot :)

skyzh and others added 8 commits October 18, 2024 17:14
@skyzh skyzh force-pushed the skyzh/split-layer-atomic branch from 4400742 to 34dd508 Compare October 18, 2024 21:15
@skyzh skyzh requested a review from a team as a code owner October 18, 2024 21:15
@skyzh skyzh requested a review from lubennikovaav October 18, 2024 21:15
@skyzh skyzh force-pushed the skyzh/split-layer-atomic branch from 34dd508 to 69d1203 Compare October 18, 2024 21:19
@skyzh skyzh removed request for a team and lubennikovaav October 18, 2024 21:19
@skyzh
resolve comments
ab762ea 
Signed-off-by: Alex Chi Z <[email protected]>
@skyzh skyzh force-pushed the skyzh/split-layer-atomic branch from 69d1203 to ab762ea Compare October 18, 2024 21:29
arpad-m
arpad-m approved these changes Oct 18, 2024
View reviewed changes
pageserver/src/tenant/timeline/compaction.rs Outdated Show resolved Hide resolved
@skyzh
discard -> drop
48da549 
Signed-off-by: Alex Chi Z <[email protected]>
@skyzh
rm discard
cc8f245 
Signed-off-by: Alex Chi Z <[email protected]>
@skyzh skyzh enabled auto-merge (squash) October 21, 2024 14:49
@skyzh skyzh merged commit aca81f5 into main Oct 21, 2024
80 checks passed
@skyzh skyzh deleted the skyzh/split-layer-atomic branch October 21, 2024 14:59
skyzh added a commit that referenced this pull request Oct 22, 2024
@skyzh
fix(pageserver): make delta split layer writer finish atomic (#9048)
64949a3 
similar to #8841, we make the
delta layer writer atomic when finishing the layers.

## Summary of changes

* `put_value` not taking discard fn anymore
* `finish` decides what layers to keep

---------

Signed-off-by: Alex Chi Z <[email protected]>
@skyzh skyzh mentioned this pull request Oct 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orca-security-us orca-security-us[bot] orca-security-us[bot] left review comments

@arpad-m arpad-m arpad-m approved these changes

@jcsp jcsp Awaiting requested review from jcsp

@problame problame Awaiting requested review from problame

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skyzh @jcsp @problame @arpad-m